Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

root: make calling Close on the root actually close everything #58

Closed
wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Member

That way, we can close and then cancel the context as a final teardown. Really, we should consider not passing a context in the first place but it is useful for requests (where we just want to abort).

That way, we can close and *then* cancel the context once we're ready to clean
everything up.
Copy link

@gammazero gammazero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is an improvement, but it continues on with what I think is a misuse of context. See comments.

However, if my concern is misplaced or requires changes outside the scope of this PR, then this should probably get merged.


var repub *Republisher
if pf != nil {
repub = NewRepublisher(parent, pf, time.Millisecond*300, time.Second*3)
repub = NewRepublisher(ctx, pf, time.Millisecond*300, time.Second*3)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably better to create and start the new republisher at the very end of this function. Otherwise, if an error occurs, the republisher remains running unless the caller cancels the context. Although it may not matter since failure to create mfs root is probably fatal.

@aschmahmann
Copy link
Contributor

This repository is no longer maintained and has been copied over to Boxo. In an effort to avoid noise and crippling in the Boxo repo from the weight of issues of the past, we are closing most issues and PRs in this repo. Please feel free to open a new issue in Boxo (and reference this issue) if resolving this issue is still critical for unblocking or improving your usecase.

You can learn more in the FAQs for the Boxo repo copying/consolidation effort.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants